-
Notifications
You must be signed in to change notification settings - Fork 121
remote ssh: add custom serverInstallPath setting #10017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
E2E Tests 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did not try it out, but the code looks good! happy to test it out on a daily build, since that seems maybe more straightforward? or, are you able to give repro steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs just a few more tweaks:
- it is pretty easy to just type something like
.positron-server-2
into the setting if you aren't reading carefully, and the behavior is undefined in this case. If the user doesn't specify an absolute path, I think we should resolve the relative path against$HOME
by default - the setting is application scoped but this feels more like a per project/machine thing b/c you probably won't have the same needs for everything you connect to. maybe change the scope? another idea would be to make the value something you can override with an env var so it could be configured on the host side
- if the user specifies a path that they can't write to and we can't make for them, what happens? (think we need better error handling for this than we have today)
- do we actually expand tildes in the setting value? (wasn't clear to me but the default uses
$HOME
; if not, the example shouldn't use them)
Yeah, good idea. I think that as this PR stands right now, the relative path will end up resolving relative to whatever the
I am generally confused about setting scopes. (It doesn't help that there's no mention of "application" in the docs!) I ended up choosing application because
...Oh, I just found the VS Code version of this setting: ![]() This looks to be application-scoped but the mapping feature is nice. Maybe I'll change the Positron setting to act like this?
We hit this code, which pops up a toast that says something like "could not install server on remote host" and opens the Output panel to the Remote SSH logs, which say positron/extensions/open-remote-ssh/src/serverSetup.ts Lines 335 to 336 in d96ae62
It might make sense to surface any bash-script-level errors better than just log entries with a generic toast, but I think that's outside the scope of this PR.
The literal text of the setting is inserted into the bash script we send to the server like |
d96ae62
to
f0d3f6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses #9013. Adds a
remoteSSH.serverInstallPath
setting that lets you customize the Positron server data directory, instead of the default~/.positron-server
.Release Notes
New Features
remoteSSH.serverInstallPath
setting lets you customize the directory for the Positron server Provide custompositron-server
install path? #9013Bug Fixes
QA Notes
The new setting should be respected. To test on this branch, you'll unfortunately need to build REH on the remote host and put it where it needs to be. It'll be easier to test on a daily build.